Skip to content

Conversation

ppannuto
Copy link
Member

This is a minimal example of what "cleanup attribute" like code would look like.

This probably isn't the exact best way to implement it (due to the "magic variable name" of ret_{param}), but it gives a sense of what it would look like.

@ppannuto ppannuto marked this pull request as draft July 16, 2025 19:49
@bradjc
Copy link
Contributor

bradjc commented Jul 16, 2025

Obviously I'm not a big fan of macros. Seems complicated compared to the goto version.

It also isn't very flexible for drivers that may need more cleanup.

@ppannuto
Copy link
Member Author

Obviously I'm not a big fan of macros. Seems complicated compared to the goto version.

I agree the macro implementation is complex (aren't they all), but I feel like the actual code is simpler and less error-prone. I messed up the incremental conversion when I'd modified just one of the allows doing just this [not that it's hard to see per se, i.e., I deleted the "top" allow call and the "top" goto label when I first tried], but it's sort of emblematic of why this goto approach is error-prone.

It also isn't very flexible for drivers that may need more cleanup.

Yes.. that was to keep the macro simpler. Some of the other flavors of this idea have you explicitly define the "destructor" as part of the invocation of the macro; it's more flexible, but also more complex. I'd imagine if we wanted to go this way keeping the simpler "only-works-for-allow" macro, since "unallow-when-done" is such a common use case, while also providing a more generic option.

@ppannuto
Copy link
Member Author

Okay, I found a much cleaner implementation of this, which matches the (current) draft specification for the next release of C, i.e., in the future defer becomes a keyword, we delete our header, and no code needs to change.

@bradjc
Copy link
Contributor

bradjc commented Jul 21, 2025

The latest version is nice, even if I don't understand the C code in the macros. But I'm worried about backwards compatibility.

@ppannuto ppannuto force-pushed the ywf-hmac-scoped-allow branch from de8d962 to 8f79efa Compare July 21, 2025 22:00
@ppannuto ppannuto changed the base branch from ywf-hmac to master July 21, 2025 22:17
@ppannuto
Copy link
Member Author

What's the worry w.r.t. backwards compatibility? This defer macro only exists in C files that explicitly include libtock/defer.h, it only changes code where it's used — so you simply write code with defer instead of goto exitN.

As for the macro, the fully expanded form basically does this:

defer { foo(); bar(); }
   // ^^^^^^^^^^^^^^ this bit doesn't change at all

// The above expands to:

auto void __DEFER_FUNCTION0(int* __DEFER_VARIABLE0SENTINEL);  // This is just a function prototype
[[ gnu::cleanup(__DEFER_FUNCTION0) ]] int __DEFER_VARIABLE0; // This declares a variable which is never used, but the compiler can see when it goes out of scope, and will call __DEFER_FUNCTION0 when it goes out of scope
auto void __DEFER_FUNCTION0(__attribute__ ((unused)) int* __DEFER_VARIABLE0SENTINEL) { foo(); bar(); }
                                                                                  // ^^^^^^^^^^^^^^^^^ here's our unchanged code, where it's now the body of a function that's being defined

@ppannuto ppannuto marked this pull request as ready for review July 21, 2025 23:00
@ppannuto
Copy link
Member Author

I rebased this on master so the CI can pick up a compiler newer than 2021.

It's now functionally a replacement PR for #532.

@ppannuto ppannuto changed the title PoC: scoped allow for hmac Yield-WaitFor: Introduce defer and update HMAC as example usage Jul 21, 2025
@bradjc
Copy link
Contributor

bradjc commented Jul 22, 2025

What's the worry w.r.t. backwards compatibility?

I rebased this on master so the CI can pick up a compiler newer than 2021.

I'm worried this won't work for users on Ubuntu 22.04 (a currently supported LTS release).

@ppannuto ppannuto force-pushed the ywf-hmac-scoped-allow branch from 6d45097 to 98ab671 Compare July 22, 2025 20:14
@ppannuto
Copy link
Member Author

I rebased this on master so the CI can pick up a compiler newer than 2021.

Turns out I didn't need to do that, just needed to do 0b840d9. I added a "legacy-compilers" CI run here that runs on the default LTS compilers for both ARM and RISCV and it seems to work without issue.

(Now, I acknowledge that #539 has some lingering issues that I'm working on, but they may actually be unrelated to compiler version; don't let issues over there poison the well here)

@bradjc
Copy link
Contributor

bradjc commented Jul 23, 2025

Oh this does work on jammy?

@ppannuto
Copy link
Member Author

Seems like it, yeah.

@bradjc
Copy link
Contributor

bradjc commented Jul 24, 2025

Before this change:

[TEST] HMAC
[0] schedule[0x1:1] @0x41659(0xd, 0x0, 0x0, 0x41601) = Ok(())
[0] function_call @0x41659(0xd, 0x0, 0x0, 0x41601)
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] read-only allow(0x40003, 0, @0x200099d4, 11) = AllowReadOnlySuccess(0x0, 0)
[0] read-only allow(0x40003, 1, @0x200098d4, 72) = AllowReadOnlySuccess(0x0, 0)
[0] read-write allow(0x40003, 2, @0x20009a50, 32) = AllowReadWriteSuccess(0x0, 0)
[0] remove_pending_upcalls[0x40003:0] = 0 upcall(s) removed
[0] subscribe(0x40003, 0, @0x41593, 0x40511) = SubscribeSuccess(0x0, 0)
[0] cmd(0x40003, 1, 0x0, 0x0) = Success
[0] schedule[0x40003:0] @0x41593(0x0, 0xb5, 0x0, 0x40511) = Ok(())
[0] yield. which: 1
[0] function_call @0x41593(0x0, 0xb5, 0x0, 0x40511)
[0] read-only allow(0x40003, 0, @0x0, 0) = AllowReadOnlySuccess(0x200099d4, 11)
[0] read-only allow(0x40003, 1, @0x0, 0) = AllowReadOnlySuccess(0x200098d4, 72)
[0] read-write allow(0x40003, 2, @0x0, 0) = AllowReadWriteSuccess(0x20009a50, 32)
[0] remove_pending_upcalls[0x1:1] = 0 upcall(s) removed
[0] subscribe(0x1, 1, @0x41659, 0x41601) = SubscribeSuccess(0x41659, 267777)
[0] read-only allow(0x1, 1, @0x20009bd0, 89) = AllowReadOnlySuccess(0x20009bd0, 13)
[0] cmd(0x1, 1, 0x59, 0x0) = Success
[0] yield. which: 1
[HMAC] Computed Output: b5055553bd5e75d4fc96bf2559d655b77a58aea8b4c4076c52f774851bba06d8

After this change:

[TEST] HMAC
[0] schedule[0x1:1] @0x415f1(0xd, 0x0, 0x0, 0x41599) = Ok(())
[0] function_call @0x415f1(0xd, 0x0, 0x0, 0x41599)
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] read-only allow(0x40003, 0, @0x200099c8, 11) = AllowReadOnlySuccess(0x0, 0)
[0] read-only allow(0x40003, 1, @0x200098c8, 72) = AllowReadOnlySuccess(0x0, 0)
[0] read-write allow(0x40003, 2, @0x20009a44, 32) = AllowReadWriteSuccess(0x0, 0)
[0] cmd(0x40003, 1, 0x0, 0x0) = Success
[0] schedule[0x40003:0] @0x0(0x0, 0xb5, 0x0, 0x0) = Ok(())
[0] yield. which: 2
[0] Yield-WaitFor: [NU] (0x0, 0xb5, 0x0)
[0] read-write allow(0x40003, 2, @0x0, 0) = AllowReadWriteSuccess(0x20009a44, 32)
[0] read-only allow(0x40003, 1, @0x0, 0) = AllowReadOnlySuccess(0x200098c8, 72)
[0] read-only allow(0x40003, 0, @0x0, 0) = AllowReadOnlySuccess(0x200099c8, 11)
[0] remove_pending_upcalls[0x1:1] = 0 upcall(s) removed
[0] subscribe(0x1, 1, @0x415f1, 0x41599) = SubscribeSuccess(0x415f1, 267673)
[0] read-only allow(0x1, 1, @0x20009bc0, 89) = AllowReadOnlySuccess(0x20009bc0, 13)
[0] cmd(0x1, 1, 0x59, 0x0) = Success
[0] yield. which: 1
[HMAC] Computed Output: b5055553bd5e75d4fc96bf2559d655b77a58aea8b4c4076c52f774851bba06d8

So looks good to me. I also forced an error into the libtock-sync hmac library and the unallows worked as expected.

bradjc
bradjc previously approved these changes Jul 24, 2025
Copy link
Contributor

@brghena brghena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks good. I like the defer addition and I do think it makes the error handling cleanup nicer.

I think defer + HMAC stuff could go together. But the CI updates seem separate?

Comment on lines 48 to 73

# Install compiler versions from the oldest current Ubuntu LTS
ci-legacy-compilers:
strategy:
matrix:
os: [ubuntu-22.04]
# The type of runner that the job will run on
runs-on: ${{ matrix.os }}

# Steps represent a sequence of tasks that will be executed as part of the job
steps:
- uses: actions/checkout@v2
with:
submodules: recursive
- uses: carlosperate/arm-none-eabi-gcc-action@v1
with:
release: '10.3-2021.07'
- run: arm-none-eabi-gcc --version
- name: setup-riscv-toolchain
run: sudo apt-get install -y gcc-riscv64-unknown-elf
- name: riscv-toolchain-version
run: riscv64-unknown-elf-gcc --version
#- name: ci-build
# run: pushd examples; ./build_all.sh || exit; popd
- name: ci-debug-build
run: pushd examples/blink; make debug RAM_START=0x20004000 FLASH_INIT=0x30051 || exit; popd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I must have forgotten to hit the button on this comment) Could we split this out to a separate PR? I think it's separate from the rest of the changes here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that doesn't need to be on this PR. I added it to prove that this works with legacy compilers, but it's unrelated. Dropped it here, will add a more CI-focused PR in a sec

bradjc
bradjc previously approved these changes Jul 24, 2025
bradjc
bradjc previously approved these changes Jul 25, 2025
@bradjc bradjc added this pull request to the merge queue Jul 29, 2025
Merged via the queue into master with commit 6395c5d Jul 29, 2025
5 of 6 checks passed
@bradjc bradjc deleted the ywf-hmac-scoped-allow branch July 29, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants